Skip to content

Enforce range non-associativity and item-modifier placement#74

Merged
StreamDemon merged 3 commits into
fix/parser-correctness-basefrom
fix/parser-strictness-leftovers
Jul 2, 2026
Merged

Enforce range non-associativity and item-modifier placement#74
StreamDemon merged 3 commits into
fix/parser-correctness-basefrom
fix/parser-strictness-leftovers

Conversation

@StreamDemon

@StreamDemon StreamDemon commented Jul 2, 2026

Copy link
Copy Markdown
Owner

Summary

The last two over-acceptances from the §16 review — closing out the correctness wave's planned scope:

  • Range non-associativity: the precedence table (docs/reference/operator-precedence.md) marks ../..= assoc "None", but the parser gave them ordinary left-associative binding powers — a..b..c silently parsed as (a..b)..c. Chained ranges now error ("range operators cannot be chained; parenthesize one side") with the span on the second range operator. (a..b)..c stays legal: the parenthesized form is explicit, and the AST cannot (and should not) distinguish it after the fact.
  • Item-modifier placement (§16): modifiers were eaten before item dispatch and never validated, so pub async struct S {}, offchain mod m;, pub impl User {}, and pub actor A {} all parsed. Per the grammar, offchain/async prefix only fn_def, and pub is absent from impl_block, actor_def, onchain_mod/event_def, and extern_block. Misplaced modifiers now error on the modifier's own token span, and the item still parses afterward so error recovery stays useful.

Stacked-PR note: targets the integration branch fix/parser-correctness-base (PR #69) — sibling of #70/#72/#73. Shares the usual conflict points with the siblings (tests-module tail, corpus array, AGENTS.md accepted bullet; and #72 adds an identical error_at helper — keep one copy at merge).

Related Issue

None (review-driven; part of the parser correctness wave tracked in PR #69).

Spec Sections Affected

None changed — implements the precedence table's "None" associativity row and §16's modifier placement as written.

Checklist

  • Code follows the Sploosh design principles (one way to do it, explicit over implicit, etc.)
  • Documentation updated in relevant docs/ pages (crates/AGENTS.md subset contract)
  • Tests added or updated
  • All build targets still compile (if applicable)
  • Spec-only PR (skip Build Targets section if checked)

Build Targets Tested

  • N/A (docs/spec only) — no codegen targets exist yet; "build" = the Rust workspace.

Test Plan

  • 4 new unit tests: range_operators_cannot_chain (a..b..c, a..=b..c, a..b..=c all rejected with the chaining message), single_and_parenthesized_ranges_parse (lo..hi, lo..=hi, (a..b)..c accepted), offchain_and_async_apply_only_to_fn_items (five rejected placements + pub offchain async fn accepted), pub_rejected_where_grammar_omits_it (rejected on impl/actor/onchain/extern; accepted on struct/enum/trait/mod/use/const/type).
  • New corpus fixture tests/corpus/ranges_modifiers.sp covering the accepted side (ranges in expressions, pub declarations, pub offchain async fn).
  • cargo fmt --all -- --check, cargo clippy --workspace --all-targets -- -D warnings, cargo test --workspace (21 tests) green locally and via scripts/docker-check.ps1 -Build (Ubuntu parity).

Summary by cubic

Enforces non-associativity for ../..= and validates item modifier placement per §16 to close parser over-acceptances. Chained ranges now error, and misplaced pub, offchain, and async are rejected with clear, anchored diagnostics; tests assert the error spans.

  • Bug Fixes

    • Reject chained ranges like a..b..c; allow (a..b)..c.
    • Restrict offchain/async to fn items.
    • Reject pub on impl, actor, onchain, and extern.
    • Diagnostics point at the second range operator or the misplaced modifier; item still parses for recovery.
  • Migration

    • Parenthesize if chaining was intended: (a..b)..c.
    • Use offchain/async only on fn, and remove pub from impl/actor/onchain/extern.

Written for commit 36c17f1. Summary will update on new commits.

Review in cubic

Two remaining over-acceptances from the section-16 review:

- The precedence table marks `..`/`..=` as non-associative, but the
  parser gave them ordinary left-associative binding powers, so
  `a..b..c` silently parsed as `(a..b)..c`. A range operand may not
  itself be an unparenthesized range: the chain is now a parse error
  ("range operators cannot be chained; parenthesize one side"), while
  `(a..b)..c` stays legal because the parenthesized form is explicit.
- Item modifiers were eaten before dispatch and never validated, so
  `pub async struct`, `offchain mod`, `pub impl`, and `pub actor` all
  parsed. Grammar places `offchain`/`async` only on fn_def, and omits
  `pub` from impl_block, actor_def, onchain_mod/event_def, and
  extern_block. Misplaced modifiers now error on the modifier's own
  span, and the item still parses afterward for error recovery.

Adds four unit tests (chained-range rejection, single/parenthesized
range acceptance, and both directions of the modifier matrix), a
ranges_modifiers.sp corpus fixture, and the crates/AGENTS.md contract
updates.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01H3CinyyqWL6SfCLrKGm3ja

@cubic-dev-ai cubic-dev-ai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

2 issues found across 4 files

Confidence score: 5/5

  • In crates/sploosh-parser/src/lib.rs, the main risk is test coverage depth: modifier-placement and range-chaining tests currently assert diagnostic text but not that errors stay anchored to the intended operator/modifier spans, so a future span regression could ship without failing tests. This looks safe to merge overall, but adding explicit span assertions for error_at(token.span, ...) and the second ../..= token before merging would de-risk the new diagnostics behavior.

Reply with feedback, questions, or to request a fix.

Re-trigger cubic

Comment thread crates/sploosh-parser/src/lib.rs Outdated
Comment thread crates/sploosh-parser/src/lib.rs Outdated
Cubic review on PR #74: both tests validated message text only, while
the PR promises span anchoring — the second range operator for chained
ranges, the modifier's own token for misplaced modifiers. The tests now
assert the error span alongside the message, so the anchoring cannot
silently regress.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01H3CinyyqWL6SfCLrKGm3ja

@cubic-dev-ai cubic-dev-ai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

0 issues found across 1 file (changes from recent commits).

Auto-approved: Adds parser diagnostics for range chaining and misplaced item modifiers, with tests. Low risk as it only rejects previously invalid syntax per the grammar.

Re-trigger cubic

@StreamDemon StreamDemon marked this pull request as ready for review July 2, 2026 07:40
@StreamDemon StreamDemon merged commit 4247268 into fix/parser-correctness-base Jul 2, 2026
@StreamDemon StreamDemon deleted the fix/parser-strictness-leftovers branch July 2, 2026 07:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant